Adding tests to postgres images project #13
Conversation
moizpgedge
force-pushed
the
Feat/PLAT-362/Adding-tests-to-postgres-images-project
branch
from
2b1097d to
49271dc
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Postgres image test suite to improve reliability and code organization. The changes fix GitHub Actions runner configuration issues, remove invalid Postgres settings, and enhance the robustness of container readiness checks.
Key changes:
- Refactored test execution flow by extracting helper functions for improved maintainability
- Updated GitHub Actions workflow to correctly map architecture names to runner labels
- Enhanced Postgres readiness checks with improved connection verification and stabilization logic
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/main.go | Major refactoring: extracted functions for parsing flags, printing headers/summaries, running tests, and waiting for containers; improved Postgres readiness verification; removed invalid lolor.node configuration |
| tests/go.mod | Simplified Go version configuration by consolidating go directive and toolchain |
| Makefile | Removed trailing newline |
| .github/workflows/test_images.yaml | Fixed architecture naming and runner label mapping to support both user-friendly and canonical architecture names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/main.go
Outdated
| // Then verify we can actually connect and query | ||
| exitCode, _, err := r.exec("psql -U postgres -d testdb -t -A -c 'SELECT 1'") | ||
| if err == nil && exitCode == 0 { | ||
| // Give PostgreSQL a moment to fully stabilize |
There was a problem hiding this comment.
While the comment explains the sleep, it should clarify why a 2-second delay is necessary beyond the successful query execution. Consider documenting what PostgreSQL state might not be fully stable after a successful query, or reference specific initialization steps that may still be in progress.
| // Give PostgreSQL a moment to fully stabilize | |
| // Give PostgreSQL a short grace period even after a successful readiness check. | |
| // Although pg_isready and a trivial SELECT can succeed, background workers, | |
| // extensions, and internal caches may still be initializing. Waiting a fixed | |
| // 2 seconds helps ensure a stable database state and reduces test flakiness | |
| // in subsequent operations that depend on a fully-initialized instance. |
moizpgedge
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/main.go
Outdated
| // Give PostgreSQL a short grace period even after a successful readiness check. | ||
| // Although pg_isready and a trivial SELECT can succeed, background workers, | ||
| // extensions, and internal caches may still be initializing. Waiting a fixed | ||
| // 2 seconds helps ensure a stable database state and reduces test flakiness | ||
| // in subsequent operations that depend on a fully-initialized instance. |
There was a problem hiding this comment.
The 2-second sleep is a magic number hardcoded in the comment. Consider making this duration configurable or defining it as a named constant (e.g., 'postgresStabilizationPeriod') to improve maintainability and make the value easier to tune if needed.
| required: true | ||
| architectures: | ||
| description: "Comma-separated list of architectures to test (amd64,arm64)" | ||
| description: "Comma-separated list of architectures to test (x86,arm)" |
There was a problem hiding this comment.
The description mentions 'x86,arm' but the code also accepts 'amd64,arm64' as valid inputs (lines 68-71). Update the description to reflect all accepted values, e.g., 'Comma-separated list of architectures to test (x86/amd64, arm/arm64)'.
| description: "Comma-separated list of architectures to test (x86,arm)" | |
| description: "Comma-separated list of architectures to test (x86/amd64, arm/arm64)" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
moizpgedge
force-pushed
the
Feat/PLAT-362/Adding-tests-to-postgres-images-project
branch
from
2288af5 to
519fcb5
Compare
|
Warning Rate limit exceeded@moizpgedge has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a new PR workflow that discovers latest image tags and runs multi-architecture tests, refactors the Go test runner into phased modular tests with Patroni support and stabilization waits, introduces a Makefile target and script option to emit latest tags, hardens initdb invocation, and expands README testing docs. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Repo as Repository
participant Make as Make / build_pgedge_images.py
participant Matrix as Matrix generator
participant Runner as Job Runner (ubuntu / ubuntu-24.04-arm)
participant Docker as Docker Engine
participant Container as Postgres/Patroni Container
participant Tests as Go test runner
participant Output as Workflow logs
GH->>Repo: checkout
GH->>Make: run `make latest-tags`
Make->>Make: scripts/build_pgedge_images.py returns tags
Make->>Matrix: build JSON matrix (tag → flavor → arch_display → runner)
GH->>Runner: schedule per matrix entry
Runner->>Repo: checkout
Runner->>Docker: pull image
Runner->>Docker: create & start container
Tests->>Container: readiness checks / waitForContainerCommand
Tests->>Container: run entrypoint & extension tests
Container-->>Tests: outputs / exit codes
Tests->>Runner: aggregate results
Runner->>Output: print structured table and pass/fail
GH->>Output: final results job prints consolidated summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/main.go:
- Around line 351-357: The call to r.cli.ContainerExecInspect is ignoring its
error so inspectResp.ExitCode may default to 0 and produce a false success;
change the inspect call to capture the error (e.g., inspectResp, err :=
r.cli.ContainerExecInspect(r.ctx, execID.ID)), check if err != nil and return or
propagate a descriptive error (or log and treat as failure) before inspecting
ExitCode; ensure execResp.Close() still runs and that you only print success and
return nil when err == nil and inspectResp.ExitCode == 0.
🧹 Nitpick comments (4)
.github/workflows/test_images.yaml (1)
137-141: Consider handling additional result states.The condition only checks for
success. Theneeds.test.resultcan also beskippedorcancelled, which would currently show as failed.♻️ Suggested improvement
- if [[ "${{ needs.test.result }}" == "success" ]]; then + result="${{ needs.test.result }}" + if [[ "$result" == "success" ]]; then echo "✅ **All tests passed!**" >> $GITHUB_STEP_SUMMARY + elif [[ "$result" == "skipped" ]]; then + echo "⏭️ **Tests were skipped.**" >> $GITHUB_STEP_SUMMARY + elif [[ "$result" == "cancelled" ]]; then + echo "🚫 **Tests were cancelled.**" >> $GITHUB_STEP_SUMMARY else echo "❌ **Some tests failed.** Check the job logs for details." >> $GITHUB_STEP_SUMMARY fitests/main.go (3)
49-68: Consider closing the Docker client.The Docker client returned by
setupDockerClient()should be closed when done to release resources.♻️ Suggested fix
func main() { image, flavor := parseFlags() printHeader(image, flavor) cli, ctx := setupDockerClient() + defer cli.Close() + defaultRunner := &DefaultEntrypointRunner{ cli: cli, ctx: ctx, image: image, }
166-173: Minor: Avoid rebuilding test suite for summary.
buildTestSuite()is called again just to count tests. Consider passing the test count as a parameter or caching the test suite.
308-311: Consider logging cleanup errors for debugging.Errors from
ContainerStopandContainerRemoveare silently ignored. While this is acceptable for best-effort cleanup, logging could help debug issues in CI.♻️ Suggested improvement
func (r *DefaultEntrypointRunner) cleanupContainer(containerID string) { - r.cli.ContainerStop(r.ctx, containerID, container.StopOptions{}) - r.cli.ContainerRemove(r.ctx, containerID, container.RemoveOptions{}) + if err := r.cli.ContainerStop(r.ctx, containerID, container.StopOptions{}); err != nil { + log.Printf("Warning: error stopping container: %v", err) + } + if err := r.cli.ContainerRemove(r.ctx, containerID, container.RemoveOptions{}); err != nil { + log.Printf("Warning: error removing container: %v", err) + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/test_images.yamlMakefileREADME.mdtests/go.modtests/main.go
💤 Files with no reviewable changes (1)
- Makefile
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~162-~162: The official name of this software platform is spelled with a capital “H”.
Context: ...D Testing The GitHub Actions workflow (.github/workflows/test_images.yaml) can be tri...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
README.md (1)
117-166: Well-structured testing documentation.The new Testing section provides comprehensive guidance for both local and CI/CD testing, including architecture limitations and workflow parameters. The documentation aligns well with the test runner implementation and workflow configuration.
tests/main.go (5)
18-23: Good use of named constant with documentation.The
postgresStabilizationPeriodconstant is well-documented, explaining why the delay is necessary. This improves maintainability over a magic number.
418-439: Robust readiness check with stabilization.The two-phase readiness check (
pg_isreadyfollowed by an actual query) combined with the stabilization period is a solid approach to reduce test flakiness.
458-466: Good defensive check for container state.Checking if the container is still running before executing commands provides clearer error messages when containers crash unexpectedly during tests.
530-536: Clean modular test suite construction.Breaking tests into
getPostgreSQLTests(),getCommonExtensionTests(), andgetStandardOnlyTests()improves readability and maintainability.
693-698: LGTM.The
expectSuccesshelper provides a clean default for tests that only need to verify the command succeeded without checking output..github/workflows/test_images.yaml (1)
65-77: Good architecture mapping with backward compatibility.The logic correctly handles both naming conventions (
x86/amd64andarm/arm64), fails fast for unknown values, and uses the validubuntu-24.04-armrunner label for ARM architecture.tests/go.mod (1)
3-3: No issues found. Go version 1.24.11 is a valid security and stability release (December 2, 2025) and correctly matches the version specified in the GitHub Actions workflow (test_images.yaml).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/main.go:
- Around line 325-366: In DefaultEntrypointRunner.waitForContainerCommand, don’t
treat ExitCode as final immediately after ContainerExecAttach/Close; after
calling r.cli.ContainerExecInspect(execID.ID) check inspectResp.Running and, if
true, sleep interval and continue the loop (only treat inspectResp.ExitCode == 0
as success when Running is false). This mirrors the safer behavior in the exec
helper (or alternatively read the attach response until EOF before inspecting),
and prevents the race where ExitCode is uninitialized while the exec is still
running.
🧹 Nitpick comments (1)
tests/main.go (1)
534-702: Excellent test suite modularization.The split into modular test builders (getPostgreSQLTests, getCommonExtensionTests, getStandardOnlyTests) improves code organization and makes it easier to maintain and extend the test suite. The
expectSuccesshelper effectively reduces duplication for tests that only need to verify successful execution.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/main.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
tests/main.go (5)
18-23: Good addition for test stability.The stabilization period constant is well-documented and helps address test flakiness by allowing background workers and extensions to fully initialize after basic readiness checks pass.
49-192: Excellent modular refactoring.The restructured main function and new helper functions improve code organization and maintainability. The separation into parseFlags, setupDockerClient, runEntrypointTests, runExtensionTests, and printSummary creates a clear test execution pipeline.
422-443: Excellent defensive approach to readiness checking.The two-stage verification (pg_isready + connection test) followed by the stabilization period is a robust solution for reducing test flakiness. The inline comments clearly explain why the grace period is necessary even after basic readiness checks pass.
This aligns well with the PR objectives to improve reliability of readiness checks.
462-471: Good defensive check for container health.The container inspection before attempting exec prevents cryptic errors when the container has stopped or crashed. The descriptive error message including the container status will help with debugging test failures.
241-323: Well-structured Patroni testing implementation.The modular approach with dedicated helper functions (createPatroniTestConfig, startPatroniContainer, cleanupContainer) makes the Patroni test flow clear and maintainable. The REST API health check using curl is appropriate for verifying Patroni initialization.
Note: The reliability of this test depends on addressing the race condition in
waitForContainerCommand(see separate comment on lines 325-366).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "-c", "track_commit_timestamp=on", | ||
| "-c", "max_replication_slots=10", | ||
| "-c", "max_wal_senders=10", | ||
| "-c", "snowflake.node=1", |
There was a problem hiding this comment.
The comment on line 380 mentions that these arguments are passed to postgres and handled by the entrypoint, but the removed lolor.node=1 configuration suggests some settings may be invalid. Verify that snowflake.node=1 is a valid PostgreSQL configuration parameter, as non-standard parameters could cause startup failures.
| "-c", "snowflake.node=1", |
mmols
left a comment
mmols
left a comment
There was a problem hiding this comment.
LGTM - i left a few comments that you can resolve before merging.
I ran this against one of the new images from #11 here as a test:
https://github.com/pgEdge/postgres-images/actions/runs/20921627844
Once this merges, can you review that PR by running the all new tags in the internal repo through this workflow?
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/pr_test_latest.yaml:
- Around line 6-18: The workflow is triggered on pull_request but pulls from an
internal GHCR image (IMAGE_REGISTRY / PACKAGE_REPOSITORY), which will fail for
forked PRs; update the test job to skip or gate runs for forks by adding a
job-level condition such as if: github.event_name != 'pull_request' ||
github.event.pull_request.head.repo.full_name == github.repository (or compare
owner with github.repository_owner) so the job only runs for PRs originating in
the same repo/org; apply the same guard to the other job block referenced (lines
~108-125).
In @README.md:
- Around line 156-160: Update the README wording to remove ambiguity by
explicitly mapping the runner labels to CPU architectures: state that
`ubuntu-latest` runs amd64 and `ubuntu-24.04-arm` runs arm64, and clarify that
the GitHub Actions workflow `.github/workflows/test_images.yaml` uses those
specific runner labels to target those architectures so readers understand the
exact mapping used by the workflow.
In @tests/main.go:
- Around line 318-359: The waitForContainerCommand function currently closes the
attach stream immediately after ContainerExecAttach, which can race with exec
completion; instead, after obtaining execResp from ContainerExecAttach (in
waitForContainerCommand), drain the attach stream (e.g., io.Copy to io.Discard
or read until EOF) before calling execResp.Close() and before calling
ContainerExecInspect; this ensures the output is fully consumed and avoids
racey/flake timeouts when checking inspectResp.ExitCode.
🧹 Nitpick comments (5)
tests/main.go (2)
221-231: Optional: align default-entrypoint readiness with the more robust readiness used for extension testsDefault entrypoint test uses only
pg_isready(Line 223-230) whilewaitForPostgresadditionally validates a real query and sleepspostgresStabilizationPeriod(Line 418-430). If you’ve seen flakes here too, consider reusing the stronger check pattern for Phase 1.Also applies to: 415-436, 18-23
283-316: Patroni config injection/quoting: avoid embedding YAML into a single-quoted echo
fmt.Sprintf("echo '%s' > /tmp/patroni.yml ...", patroniConfig)(Line 291-293) is brittle if the config ever contains'or other shell-sensitive content. Prefer a heredoc (cat <<'EOF') or base64 decode to write the file safely.Makefile (1)
71-73: latest-tags: consider running via python3 to avoid exec-bit/environment issuesIf
scripts/build_pgedge_images.pyever loses its executable bit orenv python3isn’t available,make latest-tagswill fail. A more robust pattern ispython3 ./scripts/build_pgedge_images.py --latest-tags.scripts/build_pgedge_images.py (1)
7-8: Make latest-tags output deterministic + confirm Python version compatibility
list[str]annotations require Python 3.9+; please ensure that’s an explicit project requirement (local + CI).- Consider
sorted(set(tags))to keep the output stable asall_imagesgrows/reorders.- CLI parsing currently only recognizes
--latest-tagsas argv[1]; considerargparse(or accept the flag anywhere).Also applies to: 401-418, 420-427
.github/workflows/pr_test_latest.yaml (1)
42-91: Matrix generation: prefer jq (or printf %q) over manual JSON concatenationThe current bash string concatenation for JSON is easy to break if tag values ever contain unexpected characters; using
jq -ncto construct the matrix would be more robust and easier to maintain.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/pr_test_latest.yamlMakefileREADME.mdscripts/build_pgedge_images.pytests/main.go
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 71-71: Missing required phony target "all"
(minphony)
[warning] 71-71: Missing required phony target "clean"
(minphony)
[warning] 71-71: Missing required phony target "test"
(minphony)
🪛 LanguageTool
README.md
[uncategorized] ~160-~160: The official name of this software platform is spelled with a capital “H”.
Context: ...D Testing The GitHub Actions workflow (.github/workflows/test_images.yaml) can be tri...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (18-spock5-standard, arm, standard, ubuntu-24.04-arm)
- GitHub Check: test (17-spock5-standard, arm, standard, ubuntu-24.04-arm)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
tests/main.go (1)
329-334: Verify Docker SDK API types match the module versionThis code uses
container.ExecOptionsandcontainer.ExecAttachOptions(Line 329-333, 339-340). Please double-check these types exist in the repo’s pinnedgithub.com/docker/dockerversion (compile-time).Also applies to: 339-346
README.md (1)
116-165: Go version is correctly specified and consistent across all files.Go 1.24.11 is an official release (December 2, 2025), and it is consistently used in the README (line 128), tests/go.mod, and the CI workflow (.github/workflows/test_images.yaml). No changes are needed.
Likely an incorrect or invalid review comment.
.github/workflows/pr_test_latest.yaml (1)
102-107: Go version is correctly pinned and supportedThe version
go-version: '1.24.11'matchestests/go.modexactly and is the latest patch in the Go 1.24 branch.actions/setup-go@v5fully supports Go 1.24.11. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/pr_test_latest.yaml:
- Around line 35-40: The "Get latest tags" step (id: get-tags) currently runs
TAGS=$(make latest-tags) and writes to GITHUB_OUTPUT without validating results;
update this step to check the exit status of make latest-tags and whether TAGS
is non-empty, and on failure either fail the job (exit 1 with a clear error via
echo) or set a sensible default/fallback tag before writing to GITHUB_OUTPUT, so
downstream matrix creation never receives an empty value; ensure you reference
TAGS and the GITHUB_OUTPUT write in your validation and produce a clear error
message if you choose to abort.
🧹 Nitpick comments (1)
.github/workflows/pr_test_latest.yaml (1)
42-90: Consider extracting shared matrix generation logic.The matrix generation logic (lines 44-90) is nearly identical to the logic in
test_images.yaml(lines 40-87). This duplication increases maintenance burden and risk of drift.Consider extracting this into a reusable composite action or a shared script that both workflows can invoke.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pr_test_latest.yaml.github/workflows/test_images.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (17-spock5-standard, arm, standard, ubuntu-24.04-arm)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
.github/workflows/test_images.yaml (2)
65-82: Architecture mapping logic looks good with proper error handling.The mapping from user-friendly architecture names (
arm,arm64,x86,amd64) to appropriate runners is clear and includes explicit error handling for unknown architectures. The use ofarch_displayfor consistent matrix representation is a good approach.
116-134: Results job correctly summarizes test outcomes.The renamed
resultsjob properly usesif: always()to run regardless of test outcomes, and the summary table provides useful context. The pass/fail logic based onneeds.test.resultis correct..github/workflows/pr_test_latest.yaml (3)
102-106: Verify Go version 1.24.11 exists.Same concern as in
test_images.yaml— Go version1.24.11may not exist. Ensure this is a valid, released version.
119-139: Results job correctly reports outcomes.The summary job appropriately runs with
if: always()and provides clear pass/fail messaging. However, note thatx86,armon line 132 is hardcoded rather than derived from a variable, which could become inconsistent if architectures change.
1-18: Workflow structure and permissions look appropriate.The workflow is well-structured with appropriate triggers for PRs to main. The permissions are correctly scoped to read-only for contents and packages.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/pr_test_latest.yaml:
- Line 117: The workflow hardcodes go-version: '1.24.11', which is likely
incorrect and inconsistent with the project's README; update the go-version
value (the go-version key in the GitHub Actions job) to the correct, supported
Go version used by the project or replace it with a reusable variable/matrix
(e.g., using a workflow input or matrix entry) so the CI Go version matches the
project README and other configs.
In @docker-entrypoint.sh:
- Around line 117-147: The temporary password file created in pwfile may not be
removed if initdb fails because the script exits before rm -f "$pwfile" runs;
ensure the pwfile is always cleaned by installing a trap that removes the pwfile
on EXIT (and optionally on ERR) before creating it, or by wrapping the initdb
invocation in a subshell or function that guarantees cleanup in a finally-like
block; specifically, set a trap referencing pwfile (and leave initdb_args and
initdb usage unchanged) so that cleanup runs regardless of initdb success or
failure.
🧹 Nitpick comments (2)
tests/main.go (1)
496-533: Custom command parser works for test commands but has edge case limitations.The
parseCommandfunction handles basic quote-delimited arguments correctly. However:
- Escaped quotes not handled: Strings like
echo "hello \"world\""won't parse correctly.- Unreachable fallback: Lines 528-530 are unreachable for any non-empty input string since the loop always processes characters.
For the hardcoded test commands in this file, these limitations are acceptable. If this function is reused elsewhere, consider using
shlex-style parsing or documenting these constraints..github/workflows/pr_test_latest.yaml (1)
139-153: Consider quoting$GITHUB_STEP_SUMMARYfor robustness.While
GITHUB_STEP_SUMMARYis controlled by GitHub Actions and unlikely to contain problematic characters, quoting variables is a shell best practice. You could also consolidate the redirects.Optional: Consolidated redirect with quoting
- name: Output run: | - echo "## Test Results" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "| Input | Value |" >> $GITHUB_STEP_SUMMARY - echo "|-------|-------|" >> $GITHUB_STEP_SUMMARY - echo "| Package Repository | ${{ env.PACKAGE_REPOSITORY }} |" >> $GITHUB_STEP_SUMMARY - echo "| Tags | ${{ needs.setup.outputs.tags }} |" >> $GITHUB_STEP_SUMMARY - echo "| Architectures | x86,arm |" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - - if [[ "${{ needs.test.result }}" == "success" ]]; then - echo "✅ **All tests passed!**" >> $GITHUB_STEP_SUMMARY - else - echo "❌ **Some tests failed.** Check the job logs for details." >> $GITHUB_STEP_SUMMARY - fi + { + echo "## Test Results" + echo "" + echo "| Input | Value |" + echo "|-------|-------|" + echo "| Package Repository | ${{ env.PACKAGE_REPOSITORY }} |" + echo "| Tags | ${{ needs.setup.outputs.tags }} |" + echo "| Architectures | x86,arm |" + echo "" + if [[ "${{ needs.test.result }}" == "success" ]]; then + echo "✅ **All tests passed!**" + else + echo "❌ **Some tests failed.** Check the job logs for details." + fi + } >> "$GITHUB_STEP_SUMMARY"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pr_test_latest.yamlMakefileREADME.mddocker-entrypoint.shscripts/build_pgedge_images.pytests/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/build_pgedge_images.py
🧰 Additional context used
🪛 actionlint (1.7.10)
.github/workflows/pr_test_latest.yaml
39-39: shellcheck reported issue in this script: SC2086:info:10:22: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:45:48: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:11:37: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:13:70: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:1:27: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:3:29: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:4:29: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:5:66: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:6:54: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:7:39: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:8:12: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
🪛 checkmake (0.2.2)
Makefile
[warning] 71-71: Missing required phony target "all"
(minphony)
[warning] 71-71: Missing required phony target "clean"
(minphony)
[warning] 71-71: Missing required phony target "test"
(minphony)
🪛 LanguageTool
README.md
[uncategorized] ~162-~162: The official name of this software platform is spelled with a capital “H”.
Context: ...D Testing The GitHub Actions workflow (.github/workflows/test_images.yaml) can be tri...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (17-spock5-standard, arm, standard, ubuntu-24.04-arm)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (11)
docker-entrypoint.sh (1)
131-138: LGTM - Documented limitation is acceptable.The
IFSsplit approach is a reasonable security/flexibility tradeoff. The comment at lines 132-134 clearly documents that quoted arguments with spaces aren't supported, and users needing complex arguments can pass them as function parameters instead.tests/main.go (5)
410-433: LGTM - Container lifecycle management is well-designed.The
startedflag pattern correctly prevents double cleanup. The defer at lines 412-417 handles failure cases after container creation, and the flag is only set totrueafter successful initialization. This ensures proper cleanup whetherStart()succeeds or fails.
437-458: LGTM - Stabilization period improves test reliability.The two-phase readiness check (pg_isready + SELECT 1) followed by a stabilization delay is a good pattern. The 2-second grace period allows background workers and extensions to fully initialize, reducing flakiness in subsequent tests.
287-310: LGTM - Safe heredoc approach for Patroni config.Using a heredoc with a single-quoted delimiter (
<<'PATRONI_EOF') prevents shell interpretation of the config content. SincepatroniConfigis generated from the staticcreatePatroniTestConfig()function with known content, there's no risk of the delimiter appearing in the data.
611-617: LGTM - Clean test suite organization.The modular
buildTestSuite()pattern with separate functions for PostgreSQL, common extension, and standard-only tests improves maintainability. TheexpectSuccesshelper standardizes simple pass/fail checks while allowing custom validators where output verification is needed.Also applies to: 774-779
535-550: LGTM - Defensive container state check.The container running check at lines 536-543 prevents confusing error messages when tests try to exec into a stopped container. Combined with
parseCommandfor safe argument parsing, this makes the exec flow more robust and secure.Makefile (1)
71-73: LGTM - Clean target for latest tags retrieval.The new
latest-tagstarget correctly sets the environment variable and invokes the script. The@prefix suppresses the command echo, ensuring only the script output is visible (important for downstream parsing in workflows).README.md (2)
116-170: LGTM - Comprehensive testing documentation.The Testing section clearly covers:
- What's tested (entrypoints, connectivity, extensions)
- Prerequisites and local execution
- Architecture limitations with actionable guidance
- CI/CD workflow details
This is helpful for contributors running tests locally.
128-128: > Likely an incorrect or invalid review comment..github/workflows/pr_test_latest.yaml (2)
19-27: LGTM - Proper fork PR handling.The guard at line 23 (
github.event.pull_request.head.repo.full_name == github.repository) correctly skips the workflow for forked PRs that can't access internal packages. The outputs are properly defined for downstream jobs.
52-100: LGTM - Matrix generation logic is clear and handles edge cases.The architecture-to-runner mapping (lines 81-90) correctly handles both user-friendly names (
x86,arm) and canonical names (amd64,arm64). The flavor detection with a fallback to "standard" is a sensible default.
moizpgedge
force-pushed
the
Feat/PLAT-362/Adding-tests-to-postgres-images-project
branch
from
5163311 to
2ef3665
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pr_test_latest.yaml (1)
52-100: Consider extracting duplicated matrix generation logic.This matrix generation logic (lines 52-100) is nearly identical to the logic in
test_images.yaml(lines 40-87). Consider extracting this into a reusable workflow or composite action to reduce duplication and maintenance overhead.You could create a reusable workflow or a script that both workflows call:
# Option 1: Extract to a shell script in scripts/generate_matrix.sh # Option 2: Create a composite action in .github/actions/generate-matrix/action.ymlThis would centralize the architecture mapping logic and flavor detection in one place.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pr_test_latest.yaml.github/workflows/test_images.yamldocker-entrypoint.sh
🔇 Additional comments (11)
docker-entrypoint.sh (1)
117-151: Good security hardening for initdb invocation.This refactoring improves security by:
- Using a temporary password file instead of command-line arguments (avoiding
/procexposure)- Building arguments via an array rather than
eval, preventing command injection- Adding an EXIT trap to ensure password file cleanup even on initdb failure
The documented limitation regarding quoted arguments in
POSTGRES_INITDB_ARGS(line 135-136) is an acceptable trade-off for the security benefits.One minor note: the EXIT trap pattern overwrites any existing EXIT trap. This isn't an issue currently but worth keeping in mind if future changes introduce other EXIT handlers.
.github/workflows/test_images.yaml (4)
18-22: Architecture naming improvements look good.The change from technical names (amd64, arm64) to user-friendly names (x86, arm) in the description and default values improves usability while maintaining compatibility through the mapping logic below.
65-77: Architecture mapping logic is well-implemented.The mapping handles both user-friendly names (x86, arm) and technical names (amd64, arm64) with proper error handling for unknown architectures. The runner assignment correctly maps arm variants to
ubuntu-24.04-armand x86 variants toubuntu-latest.
116-134: Results job structure looks good.The job correctly uses
if: always()to ensure the summary is generated even when tests fail, and the markdown table output to$GITHUB_STEP_SUMMARYprovides clear visibility into test configuration and results.
99-103: Go 1.23 is correct and valid. Go follows semantic versioning (major.minor.patch), so 1.24.11 is not a valid Go release version. The update from the invalid version to 1.23 is appropriate..github/workflows/pr_test_latest.yaml (6)
1-17: Well-structured workflow for PR-based testing.The workflow correctly sets up permissions (read-only for contents and packages) and uses environment variables for registry/repository configuration. The PR trigger on main branch is appropriate for pre-merge validation.
21-27: Fork detection is correctly implemented.The condition
github.event.pull_request.head.repo.full_name == github.repositoryproperly detects forked PRs, which is important since forked PRs won't have access to internal packages.
37-50: Tag retrieval with proper validation.Good error handling with
set -eand explicit validation for empty tags output. This ensures the workflow fails fast with a clear message if thelatest-tagstarget doesn't return expected data.
114-118: Go version is consistent with other workflows.The Go version 1.23 matches
test_images.yaml, ensuring consistent test execution across workflows.
131-134: Condition logic is correct.The combined condition
github.event.pull_request.head.repo.full_name == github.repository && always()correctly ensures:
- Results job is skipped for forked PRs (security)
- Results job runs for non-forked PRs even when test job fails (always shows summary)
138-153: Results output is well-formatted.The markdown summary table provides clear visibility. One minor note: the tags output references
needs.setup.outputs.tagswhich correctly displays the dynamically retrieved tags rather than hardcoding them.
…ements Security Fixes: - Fix command injection vulnerability in docker-entrypoint.sh (replace eval with safe argument parsing) - Fix command injection pattern in Go tests (safe command parsing with quote-aware parser) - Add EXIT trap to ensure password file cleanup even on initdb failure - Fix ContainerExecInspect error handling to prevent false positive test results Test Suite Improvements: - Fix container cleanup on Start() failure (self-cleanup with defer) - Replace log.Fatalf with error return in runExtensionTests to ensure summary always displays - Simplify test output: replace box-drawing tables with plain text formatting - Improve Patroni config handling with here-document to prevent command injection CI/CD Workflow Enhancements: - Add PR workflow to automatically test latest images against internal repo - Add fork PR guards to skip forked PRs without package access - Add tag validation in PR workflow (fail fast on empty tags) - Remove unnecessary Docker login/logout steps (workflows have built-in auth) - Fix Go version from invalid 1.24.11 to 1.23 in all workflows Latest Tags Functionality: - Change latest-tags to use environment variable (PGEDGE_LIST_LATEST_TAGS) - Return immutable tags with epoch from latest-tags function - Add make target to get latest tags list Documentation: - Remove CodeRabbit badge from README - Update README with explicit architecture mapping (ubuntu-latest=amd64, ubuntu-24.04-arm=arm64) - Improve test documentation with local and CI/CD instructions
moizpgedge
force-pushed
the
Feat/PLAT-362/Adding-tests-to-postgres-images-project
branch
from
2ef3665 to
7fc74ea
Compare
Summary
This PR improves the stability of the Postgres image test flow by fixing runner/architecture config issues, cleaning up an invalid config, updating build/test dependencies, and making the Postgres readiness check more reliable.
Changes
Testing
Summary by CodeRabbit
Documentation
Tests
Chores / New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.